Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughAdds configurable per-model completion limits with a process-local sliding-window limiter. The limiter applies to exec and interactive turn sessions, including provider escalation. Configuration validation, error hints, tests, and user documentation are also added. ChangesPer-model RPM limits
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CLI
participant ConfiguredTurnSessions
participant ModelRPMLimiter
participant TurnSessionProvider
CLI->>ConfiguredTurnSessions: provide profile and limiter options
ConfiguredTurnSessions->>ModelRPMLimiter: wrap the selected model session
CLI->>ModelRPMLimiter: call Stream
ModelRPMLimiter->>TurnSessionProvider: forward admitted Stream call
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds process-local per-model admission limits before provider streams, and the inspected configuration flow preserves the documented cap behavior. No actionable merge-blocking risk is established; it appears ready for normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The request cap can be silently lost when startup enters provider setup to repair an incomplete configuration. Later ordinary turns in that process can then run without the configured cap. This is a bounded admission-control defect; no broader privilege expansion was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
hello @LortuArte thanks for your contribution please rebase to main and fix conflicts |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for keeping to the slice. I checked it against the list from #390, and it's all there: modelRPM on FileConfig keyed through ResolveID, admission inside Stream before the wrapped session is entered, the exec hint in errhint beside the rate-limit case, native Go and no new dependency. The wrapper forwards Prewarm, Compact and Close untouched, escalation shares the one limiter so a switch can't reset a cap, and the TUI builds the wrapper per run, so /model keys the new model.
I mutated it to see what the tests pin. Skipping admission, letting project config relax a cap, leaving escalated sessions unlimited, keying by the raw model string, keeping a request exactly 60s old and dropping the errhint case each fail a test. Two things before it can go in:
- Merge main, as @kevincodex1 asked. It conflicts in
internal/cli/app.goandinternal/config/resolver.goafter today's merges. The red Security & Code Health check is GO-2026-6617 in Go 1.26.6'snet/http, and main moved to Go 1.26.9 today in #1124, so merging main clears that as well. - Pin the provider-command rule.
mergeModelRPM(&cfg.ModelRPM, commandConfig.ModelRPM, true)keeps a provider command from relaxing the user's cap, but nothing tests it: passingfalsethere leaves every test green. It sits next to the sandbox andcrossSessionInboundhandling for the same reason, since a provider command is an arbitrary executable, so it deserves the same kind of test.
Locally on Windows, the new RPM tests and internal/zeroruntime, internal/providers and internal/errhint pass. The one internal/config failure is TestResolveReportsExplicitMaxTurns, which reads the developer's environment; it fails the same way on this branch's base, and #1072 fixed it on main today.
Requesting changes for the two items above.
This branch has not been deployed
3 participants
Refs #390 — approved RPM-only slice. This does not implement the broader cost/TPM request.
Summary
Zero currently has no local per-model RPM admission gate at the turn-session boundary.
This change adds a process-local sliding 60-second request window keyed by resolved model ID and rejects request N+1 before the wrapped
TurnSession.Streammethod is entered.The limiter is wired into:
zero execwhile preserving the existing provider/transport selection.
FileConfig.modelRPMconfigures the cap. Unset or zero means unlimited.Project configuration may tighten an existing user limit, but may not relax it.
Admitted requests consume their slot even when the provider subsequently fails.
Scope / non-goals
This PR is intentionally limited to the scope discussed in #390:
zero execerrorIt does not add:
Separate processes have independent windows and restarting Zero clears the limiter state.
The limiter counts
TurnSession.Streamadmissions. It does not claim to count setup/prewarm traffic, compaction traffic, provider-internal retries, discovery calls, or requests made outside this boundary.See
docs/MODEL_RPM.md.Regression proof
The regression tests verify that request N+1 is rejected before entering the wrapped transport.
As a negative control, I temporarily bypassed the admission wrapper.
With the gate removed:
N+1 was not denied: <nil>The wrapper was then restored before final validation.
This verifies that the new tests fail when the behavior they are intended to protect is removed.
Concurrency result
A 100-request concurrent test with a configured RPM limit of 7 produced:
No additional admitted request reached the wrapped transport.
Validation environment
Original validation run: 2026-09-27.
Revalidated for delivery on 2026-10-03.
Before opening this draft, I rechecked the upstream repository and confirmed that
mainstill points to the exact validated base:99721c762f37cd43ac511007a5f51d1846df959eThe submitted branch was compared against the preserved validation files before publication.
Tested on:
99721c762f37cd43ac511007a5f51d1846df959ePassing checks
-race: PASSmake fmt-check: PASSgo vet ./...: PASSgo run ./cmd/zero-release build: PASSgo run ./cmd/zero-release smoke: PASSmake vulncheck: PASS (No vulnerabilities found)git diff HEAD --check: PASSgit apply --check: PASSFull-suite limitation
go test ./...does not pass completely in this validation environment.It reports 14 failing tests.
I repeated the same suite on an unmodified clean checkout of
mainat:99721c762f37cd43ac511007a5f51d1846df959eunder the same environment and network restrictions.
The same 14 test names fail on clean
main.No additional failed-test names were introduced by this patch.
The reproduced failures are:
TestAltScreenTranscriptScrollKeepsFooterFixedTestPlatformTransportRoundTripTestServeCleansStatusThroughBoundDirectoryAfterSwapTestServePreservesPreviousStatusWhenReplacementFailsTestServeRemovesSocketBoundAfterFinalRuntimeSwapTestServeSupportsReadOnlyCustomRuntimeDirectoryTestServerEndToEndTestServerPublishesDefaultStatusAfterCrashReportCreatesRuntimeDirectoryTestServerRejectsUnknownCommandTestServerSecondInstanceFailsTestTerminateAndReapDaemonProcessTestTerminateCommandKillsChildAfterLeaderExitsTestTerminateCommandStopsUnconfiguredCommandTestUnixTransportFallbackAndSocketModeThe failures include Unix-socket restrictions (
operation not permitted), daemon/process lifecycle tests and one transcript-title assertion.This comparison is evidence that the same failed-test names reproduce on the clean base in this runner.
It is not being presented as a full-suite PASS and it does not establish macOS or Windows validation.
No unrelated test was disabled or modified to hide these failures.
Static lint
make lint-staticis also not completely clean in this environment.It reports four findings:
internal/installtest/workflow_permissions_test.go:20:19— QF1001internal/proxydial/proxydial.go:67:15— QF1008internal/proxydial/proxydial.go:72:33— QF1008internal/tools/web_fetch.go:315:21— QF1008The same four findings reproduce on clean
main, and these files are not modified by this PR.I left the unrelated code unchanged.
Network isolation note
The new CLI fixtures explicitly inject a simulated/no-op MCP runtime.
For the broader validation run, external HTTP was rejected using a local deny-only HTTP proxy so tests did not make live MCP requests.
That proxy is not an OS-level network sandbox.
No credentials, live provider accounts, or real funds were used by the new fixtures.
The lint/vulnerability tooling was allowed to obtain its normal public tooling/data.
Windows revalidation — 2026-10-04
After the initial draft review, I fixed a recovery-path issue where
ModelRPMcould be lost whenResolvereturnedErrNoActiveProvider.Regression evidence:
ModelRPM[gpt-test] = 0./internal/configpassesgo test ./internal/config ./internal/cli -count=1: PASSgo test -race ./internal/config ./internal/cli -count=1: PASSgo vet ./...: PASSgo run ./cmd/zero-release build: PASSgo run ./cmd/zero-release smoke: PASSgovulncheck: PASS (No vulnerabilities found)git diff HEAD --check: PASSWindows environment:
The repository-wide
go test ./...is still not all-green in this Windows environment.On the PR branch, one full-suite run failed in
internal/toolsatTestExecCommandForegroundServerReturnsSessionAndServesHTTP; that same test then passed in isolation.I then created a detached clean worktree at the exact base commit:
99721c762f37cd43ac511007a5f51d1846df959eOn that clean base:
TestExecCommandForegroundServerReturnsSessionAndServesHTTPpassed 5/5 in isolationgo test ./...run still failed, but in a different unrelated baseline test:TestACPEndToEndPromptwithcontext deadline exceededI have not modified unrelated tests or runtime code to hide these environment/baseline failures.
Latest fix commit:
9de88d736ed1d1202e1093d958bcb44d7c1f2836Maintainer review status
The RPM implementation, recovery-path regression fix, focused tests, race validation, vet, build, smoke, formatting, vulnerability scan and diff hygiene all pass locally.
The repository-wide Windows suite is not all-green, but the clean base also produces unrelated full-suite failures under the same environment.
The PR is ready for project CI and maintainer review. I have intentionally left unrelated baseline/environment failures unchanged.
Disclosure
AI-assisted implementation and local verification.
This PR does not claim maintainer endorsement, Zero adoption of AEGIS, or production certification.
Summary by CodeRabbit
modelRPM. Limits apply to interactive turns andzero exec; requests over the limit are rejected locally with guidance on when to retry.